Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat(clustering/sync): flatten and report validation error #14262

Merged
merged 19 commits into from
Feb 14, 2025
Merged

feat(clustering/sync): flatten and report validation error #14262

merged 19 commits into from
Feb 14, 2025

Conversation

chobits
Copy link
Contributor

@chobits chobits commented Feb 12, 2025 •
edited
Loading

Summary

  1. flatten validation error for sync deltas
  2. call RPC notify to API kong.sync.v2.notify_validation_error (spec: https://github.com/Kong/openrpc_specfiles/pull/18/)
  3. fix bug that CP will crash if DP calls a non-existent RPC notify: KAG-6381

Checklist

  • The Pull Request has tests
  • A changelog file has been created under changelog/unreleased/kong or skip-changelog label added on PR if changelog is unnecessary. README.md
  • There is a user-facing docs PR against https://github.com/Kong/docs.konghq.com - PUT DOCS PR HERE

Issue reference

Fix KAG-6351 KAG-5898 KAG-6381

@chobits chobits requested review from flrgh and chronolaw and removed request for flrgh February 12, 2025 03:28
@github-actions github-actions bot added core/clustering core/db cherry-pick kong-ee schedule this PR for cherry-picking to kong/kong-ee labels Feb 12, 2025
@chobits chobits requested a review from flrgh February 12, 2025 03:29
chronolaw
chronolaw reviewed Feb 12, 2025
View reviewed changes
kong/clustering/services/sync/rpc.lua Outdated Show resolved Hide resolved
kong/clustering/services/sync/validate.lua Outdated Show resolved Hide resolved
@chronolaw
Copy link
Contributor

We should open a same PR in CE.

@chobits
Copy link
Contributor Author

chobits commented Feb 12, 2025

We should open a same PR in CE.

this is a CE pr now

@chobits
Copy link
Contributor Author

chobits commented Feb 12, 2025

For pre-running ee test cases, see this pr: https://github.com/Kong/kong-ee/pull/11363

chronolaw
chronolaw approved these changes Feb 13, 2025
View reviewed changes
Copy link
Contributor

@chronolaw chronolaw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this PR is good enough, need another reviewer to confirm.

@chobits chobits requested a review from ADD-SP February 13, 2025 05:34
lhanjian
lhanjian requested changes Feb 13, 2025
View reviewed changes
Copy link
Contributor

@lhanjian lhanjian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no e,

is latest_version

kong/clustering/services/sync/rpc.lua Outdated Show resolved Hide resolved
StarlightIbuki
StarlightIbuki approved these changes Feb 13, 2025
View reviewed changes
kong/clustering/services/sync/rpc.lua
-- example: { version = <latest version of deltas>, error = <flatten error>, }
manager.callbacks:register("kong.sync.v2.notify_validation_error", function(node_id, msg)
ngx_log(ngx_DEBUG, "[kong.sync.v2] received validation error")
-- TODO: We need a better error handling method, it might report this error
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we associate the error with a specific call?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes we could. but it might be a TODO ticket later. Need to have an idea how to recover this with our product manager.

@chobits
Copy link
Contributor Author

chobits commented Feb 13, 2025

no e,

is latest_version

all fixed.

@chobits
Copy link
Contributor Author

chobits commented Feb 13, 2025

example that cp received from dp, the payload is as follows:

{
  error = {
    code = 21,
    fields = {},
    flattened_errors = { {
        entity_id = "8ef87a74-4e7f-4996-a92f-6540ee8d50bf",
        entity_name = "service-001",
        entity_type = "service",
        errors = { {
            field = "host",
            message = "required field missing",
            type = "field"
          } }
      } },
    message = "sync deltas is invalid: {}",
    name = "sync deltas parse failure",
    source = "kong.clustering.services.sync.validate.validate_deltas"
  },
  version = "v02_test_version"
}

@ADD-SP ADD-SP merged commit 417a7f9 into master Feb 14, 2025
26 checks passed
@ADD-SP ADD-SP deleted the feat/sync_error_report branch February 14, 2025 01:35
@ADD-SP ADD-SP added the incomplete-cherry-pick A cherry-pick was incomplete and needs manual intervention label Feb 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lhanjian lhanjian lhanjian approved these changes

@chronolaw chronolaw chronolaw approved these changes

@ADD-SP ADD-SP ADD-SP approved these changes

@StarlightIbuki StarlightIbuki StarlightIbuki approved these changes

@flrgh flrgh Awaiting requested review from flrgh

Assignees

@chobits chobits

Labels
cherry-pick kong-ee schedule this PR for cherry-picking to kong/kong-ee core/clustering core/db incomplete-cherry-pick A cherry-pick was incomplete and needs manual intervention size/L skip-changelog
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@chobits @chronolaw @lhanjian @ADD-SP @StarlightIbuki